-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Render project workflows on the server #1809
Conversation
Tests are broken and the code needs re-organising, but this works, which is very promising. |
packages/app-project/src/screens/ProjectHomePage/components/Hero/HeroContainer.js
Outdated
Show resolved
Hide resolved
1314c84
to
fec82c4
Compare
packages/app-project/src/screens/ProjectHomePage/components/Hero/HeroContainer.js
Outdated
Show resolved
Hide resolved
This is neat. NextJS serialises the project and workflows data as JSON, then requests |
d0845d6
to
85f3518
Compare
273053f
to
f43f427
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't really had index.js
files hold code before, just handle import/export of modules. I don't think I would think to look there for that. I would prefer organizationally that the async requests added to the packages/app-project/pages/projects/[owner]/[project]/index.js
file be put elsewhere ideally with specs. We've used folders called helpers
elsewhere to contain one off functions with specs.
...ens/ProjectHomePage/components/Hero/components/WorkflowSelector/WorkflowSelectorContainer.js
Outdated
Show resolved
Hide resolved
I'm hoping #1823 will make things cleaner by moving the common page props, like the project, into a shared function. However, with NextJS 9.3 and up, we're going to have to add functions to individual pages. It's probably a good idea to keep those functions short. I wasn't sure how to test the page props here, without just duplicating the |
f7ef425
to
6d5d637
Compare
I've tidied up getting the page props from the request. #1823 should allow us to replace the duplicated project fetch (once in const { env } = query
const { props: defaultProps } = await getDefaultPageProps({ params, query, req, res })
const language = defaultProps.initialState.project.primary_language
const { active_workflows, default_workflow } = defaultProps.initialState.project.links
const workflows = fetchWorkflowsHelper(language, active_workflows, default_workflow, env)
const props = Object.assign({}, { workflows }, defaultProps)
return { props } |
For the content pages app, I've experimented with putting the page data fetching functions under I'm not 100% sold on that approach because I wouldn't necessarily look in |
6d5d637
to
eb7132f
Compare
3c64a30
to
99ca725
Compare
I ended up copying the tests for |
e3bc04b
to
0204df7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking, but I don't think it's clear upon reading the WorkflowSelector
code that it's contingent on the user loading state, not the workflow loading state. I think given the name of the component, one would assume it's the workflow. I think this could be better named to avoid this confusion.
Also, what happens now when the workflow fails to load? The UI has been changed to only display certain UI messaging based on user load state, not workflow, so what if the workflow request fails?
packages/app-project/src/screens/ProjectHomePage/ProjectHomePage.js
Outdated
Show resolved
Hide resolved
const workflows = await fetchWorkflowsHelper(language, active_workflows, default_workflow, env) | ||
const props = { workflows } | ||
return ({ props }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if these requests fail? What kind of handling should these have for different scenarios, say 404 vs 500s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. What happens at the moment? Doesn't the component display a message about workflows failing to load?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could send a project workflows error state back with the page props. The generated HTML might be cached across page refreshes, which could be a problem if it's a temporary error.
Uncaught errors on the server will generate a 500 response, but that won't be cached. The error page is customisable, so setting up a project error page might be a good first issue. That ties into having a custom 404 page too (see #1694.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NextJS recommend not handling server-side errors, although there's next-to-no official documentation about that, just discussions about best practice. vercel/next.js#11644 has some discussion about error-handling.
If we do handle errors, and send back a 200 response with a project home page, then it's worth noting that this can be indexed by search engines (as well as cached by browsers.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next 10 adds notFound
and redirect
keys to the returned object from getServerSideProps
. That could be useful.
Also worth noting that the workflows request can't 404. notFound
would only be needed when requesting a specific resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with punting this issue for version 10 if it has a better API for it. Let's track in an issue?
1348c8f
to
079dffd
Compare
d582ead
to
d78c1b7
Compare
406e19e
to
89728a2
Compare
I've done some reading up on server-side errors in NextJS. I think we have two choices.
EDIT: 200 responses can be indexed by search engines too. |
Request workflows for the home page in getServerSideProps, then pass the workflow data down to the Hero component as a prop.
Add the logged-in user to the workflow selector from the mobx store. Pass the user loading state down as a prop.
Pass down workflow data as a prop, without loading status.
Remove the getProjectWorkflowsHelper. Get the workflow language from the project.
Mock a linked workflow for a mock request slug and test the staging and production APIs.
`loadingState` for the workflow selector doesn't really tell us what is being loaded from the API. `userReadyState` is more accurate and consistent with `subjectReadyState` used elsewhere in the code.
Add workflows prop types to the home page, and home page container. Default workflows to an empty array.
b781775
to
270a674
Compare
Get workflows from the project with getDefaultPageProps, so that project workflows are also available on the Classify page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm satisfied having getServerSideProps
in the index file of the pages for now. However, one recommendation I have is to at least add basic smoke unit tests for these and this would I think be sufficient to signal to another dev that there's code in these files.
Any uncaught error in getServerSideProps returns a 500 response and the app's error page. The default error page is a blank page saying 'An unexpected error has occurred' (see the screenshot in #1826.) NextJS recommends letting Next handle your server-side errors and set up a custom error page. This seems like the appropriate response, to me, if Panoptes is down.
This seems like the right choice to me too.
In #1964 I’ve figured how to customise the error message. I haven’t experimented much with it, but that might work for 500 errors too. |
One thing I haven’t figured out: how to add tests to pages. Every JS file under |
This might work. Put the page tests in |
Request workflows in
getDefaultPageProps
, then pass the workflow data down to the Hero component as a prop. Wait for user login to complete before displaying the list of workflows in the browser.This is now based off #1823 so that
getServerSideProps
is consistent across branches.Package:
app-project
Closes #1803.
Fixes #1827.
Review Checklist
General
Components
Apps
yarn panic && yarn bootstrap
ordocker-compose up --build
and app works as expected?Publishing
Post-merging